Respect phpstan level in InvalidKeyInArrayDimFetchRule #4166
Conversation
VincentLanglet
force-pushed
the
invalidKey
branch
2 times, most recently
from
b32f7fe to
2caa239
Compare
There was a problem hiding this comment.
If you're doing something to respect levels better, you need to add some cases to LevelsIntegrationTest. You can just add the "topic" php file and run the test, it will generate JSONs with the current result.
It'd actually be great to see the current behaviour in the first commit and then see how things get corrected in the second commit with the code changes.
The expectations are:
- level < 7 - report things that are always wrong
- level 7 - report things that might be wrong, like
int|stringonly whereintis expected - level 8 - same but for
int|null - level 9 - explicit mixed
- level 10 - implicit (untyped) mixed
|
The best way to get LevelsIntegrationTest results fast is to comment out all the other "topics" when you're working on a single topic. Otherwise it takes a long time for the tests to run. Also you could add some upload-artifact action call to |
VincentLanglet
force-pushed
the
invalidKey
branch
4 times, most recently
from
cc2bcaf to
da3e7fd
Compare
VincentLanglet
force-pushed
the
invalidKey
branch
from
da3e7fd to
5e15e2e
Compare
VincentLanglet
force-pushed
the
invalidKey
branch
from
6c78056 to
ae06b48
Compare
VincentLanglet
force-pushed
the
invalidKey
branch
from
891baf8 to
38c316f
Compare
|
Ok, so here you have
Then last commit was just to fix the lint 7.4. |
| $varType = $this->ruleLevelHelper->findTypeToCheck( | ||
| $scope, | ||
| $node->var, | ||
| '', | ||
| static fn (Type $varType): bool => $varType->isArray()->no() || AllowedArrayKeysTypes::getType()->isSuperTypeOf($dimensionType)->yes(), | ||
| static fn (Type $varType): bool => $varType->isArray()->no(), | ||
| )->getType(); | ||
|
|
||
| if ($varType instanceof ErrorType || $varType->isArray()->no()) { |
There was a problem hiding this comment.
What feels wrong here is that you're doing separate var and dim checks.
If the var check returns ErrorType (because we're on level 3 and not 7+ for example), you're not even going to run the dim check - which might actually report something wrong on level 3+.
There was a problem hiding this comment.
It's already the case no ? The rule is already reporting nothing about $dimensionType if $varType is an errortype or not an array. And to me it make sens, we shouldn't report an invalid array offset, if the variable is not an array.
varType can be an ErrorType
- when it's mixed on level 7- ; we shouldn't report anything about the offset since we dunno the var type
- when it's not existing class ; not an array
- when it's an object and union is not checked ; not an array
- I'm didn't found other case in RuleLevelHelper.
Do you have an exemple of case which should be reported on level 3 and is not with the current implement ? Cause all the existing tests are green, and I really think the change is correct as showed by the integration tests AND necessary bugfix for #4012
Extracted from #4012
It will be easier to review the CastArrayKeyRule after